Allow support channel settings to be either channel name OR channel ID #613
Conversation
This gets rid of some duplicate code and makes it simpler to add additional support channels in future.
Most slack functions will accept either a channel name or a channel ID. We always use a channel ID for support channels for the few that require it. For readability, our settings have used channel names, but the disadvantage is that if a channel name changes, bennettbot will no longer be able to post to it. This allows settings to be either channel name or channel ID. If we decide to change them to channel IDs in prod, this means channel names can be changed with no interruption to how the support keywords work.
rebkwok
changed the title
| @@ -290,7 +317,6 @@ def repost_support_request_to_channel(event, say, ack, keyword, channel_id): | |||
| # an exception; if this happens, then the user is editing something | |||
| # other than the keyword in the message, and we don't need to repost | |||
| # it again. We let the default error handler will deal with it. | |||
| reaction = {"tech-support": "sos", "bennett-admins": "flamingo"}[keyword] | |||
There was a problem hiding this comment.
reactions_add takes the emoji names without being wrapped by colons, so the new support_config should store them without the colons (lines 122 and 138)
Currently Slack will complain about not being able to add a reaction:
Unexpected error: SlackApiError("The request to the Slack API failed. (url: https://slack.com/api/reactions.add)\nThe server responded with: {'ok': False, 'error': 'invalid_name'}") while responding to message Add the reaction tech-support
There was a problem hiding this comment.
Good catch, thanks! Although I feel sure that I tested this...
There was a problem hiding this comment.
Hah. No, I tested the bad job, which just reposts to tech-support
bennettbot/bot.py
Outdated
| # Get the channel ID by channel name. If it doesn't exist, assume the setting is | ||
| # already the channel ID | ||
| "channel_id": channels.get( | ||
| settings.SLACK_TECH_SUPPORT_CHANNEL, settings.SLACK_TECH_SUPPORT_CHANNEL | ||
| ), |
There was a problem hiding this comment.
How would this affect the message query in check_messages in MessageChecker, if the setting is already the channel ID? (Line 259, dispatcher.py)
There was a problem hiding this comment.
MessageChecker is fine, it can take either the channel name or the ID
There was a problem hiding this comment.
Although really, bot and dispatcher should share the same support config. I'll have a look at doing that as well as fixing the emoji
Both the bot and the message checker in the dispatcher use elements of the support config, so move it out into a shared module. I'm not greatly happy about having a config.py that this lives in, but I don't want to pollute settings.py with functions that build config, and I don't want dispatcher.py to be dependent on bot.py Also fixes a bug with the reaction emoji config, and uses "support_channel" as the key in the support config to be more explicit about which channel we're referring to in functions that use this config.
bennettbot/config.py
Outdated
| # available, otherwise we default to using the setting value (on the | ||
| # assumption that the setting value is the channel name). |
There was a problem hiding this comment.
Looks good! I guess if we do switch to using channel IDs in the settings, we will have to update this comment in that PR?
There was a problem hiding this comment.
No - although I think I haven't said it very clearly, and I think I actually meant we assume the setting value is already the channel ID (not name). Currently the setting can be either the name or the ID; we first assume the setting is a channel name, and use it to retrieve the ID from the channels if the channels dict is available.
In the bot case, if if it can't be retrieved, we assume it's the ID already, and code in the handler registration checks that the IDs are valid.
In the dispatcher case, we don't have the dict of channel names to IDs available, so we default to using whatever the setting is, which may be a name or an ID.
I'll reword this comment
The bennett admins channel name was recently changed, which required a change to the production environment variable, because our settings used channel names and not channel ids. There are benefits to being able to use both.
Most slack functions will accept either a channel name or a channel ID. We always use a channel ID for support channels for the few functions that require it. For readability, our settings have used channel names, but the disadvantage is that if a channel name changes, bennettbot will no longer be able to post to it.
This allows settings to be either channel name or channel ID. If we decide to change them to channel IDs in prod, this means channel names can be changed with no interruption to how the support keywords work.
Most of the changes in this PR are actually just some refactoring of how the support channel config is set up.